Conversation
Set-ReFSDedupSchedule replaces a volume's schedule rather than adding to it, so the loop that called it once per chosen time left only the last one behind - while the approved plan, the progress lines during the run and the closing reminder all promised every time. Established three ways. The documented signature takes -Start <DateTime>, mandatory and singular. An array of two times is refused outright. And three consecutive calls on a freshly created Dev Drive each left exactly one schedule and one trigger, holding that call's own time. - ask for one daily start time, still as HH:MM so 18:30 stays possible - drop Resolve-DedupTimeListInput and Format-DedupTimeList; the daily question now uses Resolve-DedupTimeInput, as the weekly one already did - write the schedule with a single call, pinned by a syntax-tree assertion that no loop of either shape encloses it - compare the start time in the read-back, which is what would have caught this, rendering the DateTime the cmdlet returns invariantly - tell the user Task Scheduler accepts further triggers by hand, that nothing here reports them back, that the next schedule written removes them, and that whether the optimization runs on one is unconfirmed - stop reading the documentation's "one or more volumes" as "one or more schedules" Closes #94 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Reviewed by free · Input: 106.8K · Output: 12.2K · Cached: 405.6K |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #94.
The defect
The script asked for up to four daily optimization start times, then called
Set-ReFSDedupScheduleonce per time in a loop. That cmdlet replaces a volume's schedule rather than adding to it, so only the last time survived — while the plan the user approved, the progress lines during the run and the closing reminder all promised every time.Resolve-DedupReadBackVerdictdid not catch it: it compared mode, format and level, never the time.Why one time, established three ways
Set-ReFSDedupScheduledeclares-Start <DateTime>— mandatory, singular, not an array. The-Volume <String>description is the one that says "Enter one or more volume IDs … Separate multiple volumes with a comma"; that phrase is about volumes, and reading it as being about schedules is where the multi-schedule accommodation in this codebase came from.Enable-ReFSDeduprather than bySet-ReFSDedupSchedule, and thatGet-ReFSDedupSchedulereturnsStartas aSystem.DateTimecarrying the date the schedule was written.The same loop is present unchanged as far back as
a794ebb, so this is not a regression — it never worked.What changed
HH:MMso 18:30 remains possible.Resolve-DedupTimeListInputandFormat-DedupTimeListare gone. The daily question now usesResolve-DedupTimeInput, which the weekly start-time question already used; with one time,Format-DedupTimeListhad become a function that returned its own argument.Set-ReFSDedupScheduleappears exactly once, and no ancestor of that call is a loop statement or a script block — so neitherforeachnorForEach-Objectcan bring the defect back.DateTimethe cmdlet returns is rendered invariantly byFormat-DedupScheduleStart; a value that is not a date and time answers empty, and the verdict names that gap rather than passing over it.-Daysa quoted comma-joined string,-StartanHH:MMstring,-CpuPercentagea plain number. Building-Startas a realDateTimewas considered and dropped — string-to-DateTimeconversion gives the identical correct result underen-US,uk-UA,fi-FI,de-DE,th-THandar-SA.Get-DedupVolumeReport's docstring and one test name stopped resting on the "one or more" misreading.Checks
Parse OK;
Invoke-ScriptAnalyzer -Path . -Recurseno findings; 665 tests pass (658 before the review round below). The pre-commit hook ran the same three.The new syntax-tree assertion was proved to have teeth by reintroducing the defect on a scratch copy, in both shapes:
Review
An independent review raised 17 points; 13 were applied. The three that mattered:
1, so the branch printing the daily-time prompt never ran and itsShould -Not -Invokeassertions passed on nothing — and it had replaced a test that did drive that branch. It now answers2first and asserts the prompt was printed.LoopStatementAst, so$times | ForEach-Object { … }would have evaded it while the comment claimed it caught any loop. It now walks the parent chain for both shapes.Also applied:
$baseScheduleParamsrenamed to$scheduleParams(nothing is "base" without the clone and the loop); theScheduled the daily jobline removed, since it claimed success from a call having returned and the read-back two lines later reports it properly; the menu no longer offers "these times" where there is one; a test now pins thatResolve-DedupTimeInputandFormat-DedupScheduleStartrenderHH:MMidentically, a coupling nothing held before; several comments shortened.Declined: that an unreadable start time deserves its own header rather than the "does not report back what was asked for" one. The difference line directly beneath already says "the volume reported none that could be read", and splitting the header for a case that arises only if the returned type changes adds structure without adding clarity.
Left open, recorded rather than guessed: whether
Get-ReFSDedupSchedulecan return a scrub entry beside the daily one — the probe called onlySet-ReFSDedupSchedule— and whetherStartcomes back as aDateTimeon every Windows build, which is one measurement on one build.🤖 Generated with Claude Code